Skip to content

RDK-61247: IP caching and refresh logic#310

Open
tukken-comcast wants to merge 8 commits into
developfrom
topic/RDK-61488
Open

RDK-61247: IP caching and refresh logic#310
tukken-comcast wants to merge 8 commits into
developfrom
topic/RDK-61488

Conversation

@tukken-comcast
Copy link
Copy Markdown

Reason for change: Implement IP caching and refresh logic
Test Procedure: See ticket
Priority: P1
Risk: Medium

…ation

- Subscribe to notify::options on NMDhcpConfig in all three wiring sites
  (device-added, startup walk, ip4/ip6ConfigChangedCb) so dhcpserver stays
  current across mid-lease renewals; remove stale TODO comment
- Replace IpFamilyCache::globalAddresses (std::set) + prefix (uint32_t)
  with std::map<string,uint32_t> so toIPAddress() always projects ipaddress
  and prefix from the same entry
- Remove dead GnomeNetworkManagerEvents::onAddressChangeCb() definition
  and declaration, superseded by the cache-diff path in refreshIpFamilyCache
…tection

Extract isIPv6LinkLocal() helper into NetworkManagerImplementation.h and use
it at all four call sites. Also remove now-unused #include <set>.
Update cache insert calls in gnome and rdk proxy fallback paths to use
the map<string, uint32_t> API (insert({addr, prefix})) and remove the
now-absent top-level c->prefix field assignments.
refreshIpFamilyCache() was reading addresses from
nm_active_connection_get_ip4/6_config(), which returns NULL on platforms
like xione-uk where NetworkManager does not manage the IP configuration
directly. The signal handlers (ip4ChangedCb, ip6ChangedCb) are connected
to the device-level NMIPConfig objects, so the cache must also read from
nm_device_get_ip4/6_config() to see the same addresses that triggered
the notification.

This mismatch caused the cache to remain empty and no IP_ACQUIRED events
were ever emitted despite signals firing correctly.
nm_ip_config_get_nameservers() returns a NULL-terminated strv. In newer
libnm versions this is guaranteed non-NULL even when empty (returns a
pointer to {NULL}). The previous code accessed dnsArr[1] unconditionally
after checking dnsArr non-NULL, which reads past the single-element
allocation when there are zero DNS servers configured.

On the xione-uk platform, the device-level IP config has addresses from
the kernel but no DNS servers via NetworkManager, so the returned strv
is empty. Reading dnsArr[1] past the allocation boundary causes SIGSEGV
on this embedded platform.

Fix: only access dnsArr[1] when dnsArr[0] is confirmed non-NULL. Also
use the correct const type to avoid the C-style cast.
Three changes fix the missing IP_LOST events when disconnecting:

1. Call refreshIpFamilyCache in deviceStateChangeCb before reporting
   INTERFACE_LINK_DOWN or INTERFACE_REMOVED. When NM disconnects a
   device, it batches State and Ip4Config/Ip6Config property changes
   into a single D-Bus PropertiesChanged signal. libnm updates all
   properties atomically then emits notify signals in arbitrary order.
   If notify::state fires before notify::ip4-config, the cache was
   being cleared (by ReportInterfaceStateChange) before
   refreshIpFamilyCache could diff against it. By explicitly calling
   refreshIpFamilyCache from the state callback, the diff runs while
   the cache still has the old addresses, producing IP_LOST events
   through the canonical path with proper logging.

2. ReportInterfaceStateChange now emits IP_LOST for every cached address
   before clearing the cache. This is a safety net: if
   refreshIpFamilyCache already handled the transition (because
   notify::ip4-config fired first), the cache will be empty and this
   emits nothing. If it didn't, this catches any remaining addresses.

3. Move the nm_device_get_ip4/6_config() read outside the if(conn) gate
   in refreshIpFamilyCache. The device-level IP config does not require
   an active connection, so the cache can detect address presence or
   absence during teardown when the active connection is already gone.
- refreshIpFamilyCache now checks device state: when
  devState <= NM_DEVICE_STATE_DISCONNECTED, it skips the libnm read
  so newCache is empty and the diff emits IP_LOST for all cached
  addresses. This eliminates the signal-ordering dead zone where
  addresses were still present in libnm but about to be cleared.

- Remove IP_LOST emission from ReportInterfaceStateChange (was
  duplicating the cache-clear + emit logic). That function now only
  manages interface state (connected flags, default interface,
  connectivity monitor).

- Move the authoritative 'IP acquired:'/'IP lost:' log line into
  ReportIPAddressChange, which is the single guaranteed call site
  for every IP event regardless of origin.

- Remove the now-redundant 'IP acquired:'/'IP lost:' prints from
  refreshIpFamilyCache's diff section.

Verified: disconnect produces exactly one IP_LOST per cached address,
no spurious IP_ACQUIRED, and IP_LOST events precede LINK_DOWN.
Copilot AI review requested due to automatic review settings May 13, 2026 13:06
@tukken-comcast tukken-comcast requested a review from a team as a code owner May 13, 2026 13:06
@rdkcmf-jenkins
Copy link
Copy Markdown
Contributor

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

  • Protex Server Path: /home/blackduck/github/networkmanager/310/rdkcentral/networkmanager

  • Commit: 90e059f

Report detail: gist'

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Implements per-interface/per-IP-family IP caching and refresh to serve IP settings from event-driven state (instead of repeatedly querying NetworkManager/IARM), and improves correctness around address change notifications (including better link-local detection).

Changes:

  • Introduces IpFamilyCache (IPv4/IPv6, eth/wlan) with a mutex and uses it in GetIPSettings() fast-paths.
  • Adds libnm-driven refresh logic in GNOME events to diff address sets and emit acquired/lost notifications.
  • Normalizes IPv6 link-local detection via a shared isIPv6LinkLocal() helper and updates logging for IP change events.

Reviewed changes

Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
plugin/rdk/NetworkManagerRDKProxy.cpp Serves IP settings from new cache and populates cache from IARM results.
plugin/NetworkManagerImplementation.h Adds IpFamilyCache, isIPv6LinkLocal(), and cache members + mutex.
plugin/NetworkManagerImplementation.cpp Removes old per-interface address fields usage; updates IP change logging.
plugin/gnome/NetworkManagerGnomeProxy.cpp Uses event-driven cache when available; updates fallback caching and IPv6 link-local detection.
plugin/gnome/NetworkManagerGnomeEvents.h Declares refreshIpFamilyCache() API for cache refresh/diffing.
plugin/gnome/NetworkManagerGnomeEvents.cpp Implements cache refresh from libnm state, hooks additional signals, emits acquired/lost diffs.
plugin/gnome/gdbus/NetworkManagerGdbusUtils.cpp Switches IPv6 link-local detection to isIPv6LinkLocal().
plugin/gnome/gdbus/NetworkManagerGdbusEvent.cpp Switches IPv6 link-local detection to isIPv6LinkLocal().
plugin/gnome/gdbus/NetworkManagerGdbusClient.cpp Uses cache fast-path and attempts to populate cache in fallback path.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +1570 to +1572
c->globalAddresses.insert(result.ipaddress);
c->linkLocalAddress = result.ula;
c->prefix = result.prefix;
@@ -654,17 +654,13 @@ namespace WPEFramework
{
@rdkcmf-jenkins
Copy link
Copy Markdown
Contributor

b'## Blackduck scan failure details

Summary: 0 violations, 0 files pending approval, 1 file pending identification.

  • Protex Server Path: /home/blackduck/github/networkmanager/310/rdkcentral/networkmanager

  • Commit: 90e059f

Report detail: gist'

IpFamilyCache m_wlanIPv4Cache;
IpFamilyCache m_ethIPv6Cache;
IpFamilyCache m_wlanIPv6Cache;
mutable std::mutex m_ipCacheMutex;
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use map, some thing like :
map<std::string, IpFamilyCache> ipAddrCache
This can reduce the code that we are adding to access the correct cache.
All the if loops in the implementations can be optimized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants